fix(claude-ops): resolve built-in descriptions the inventory left unresolved - #5619

@codex review

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Claude finished @kyle-sexton's task in 44s —— View job

Security review — PR #5619

• Gather context (PR diff, changed files)
• Run /review:security-review for PR fix(claude-ops): resolve built-in descriptions the inventory left unresolved #5619
• Post findings

No security issues found.

Reviewed all 10 changed files (origin/main...HEAD, head 337bf91). The functional changes are confined to plugins/claude-ops:

• inventory.py — static-analysis parsing logic over the locally installed Claude Code CLI bundle (template/fallback/parenthesized-expression resolution, module-scoped identifier resolution for the bundled-module layout). No new file I/O, subprocess, eval/exec, deserialization, or network calls were introduced; all new code operates on an in-memory string already read by existing code paths.
• discover.py — adds camelCase word-splitting and an extra scored text field to the TF-IDF bag used for overlap detection. Pure text scoring, no new I/O.
• native_drift.py — adds an unresolved-description drift item derived from integrity.undetermined.description_unresolved, filed through the existing key-based path. No new external input handling.
• extraction.md, native-drift.md, CHANGELOG.md, plugin.json — documentation and version-bump only.
• test_*.py — test coverage only.

None of the changed files are CLAUDE.md/AGENTS.md/rules/skill/agent-body files, so the instruction-surface-deletion check doesn't apply. No injection surfaces (command/SQL/path/template), no credential/token handling, and no authorization/access-control logic in this diff.

Claude finished @kyle-sexton's task in 6m 36s —— View job

Code review complete

• Load /review:code-review skill and instructions
• Gather PR diff and changed files
• Review inventory.py resolution logic (call-with-arguments follow-through, param shadowing, module-scoped identifier resolution, ||/?? fallbacks, .join() arrays)
• Review discover.py PascalCase splitting and user_facing_name scoring
• Review native_drift.py drift intake changes
• Review test coverage
• Post findings

Summary

Reviewed the diff against origin/main (10 files, +779/-72). The core of this PR is a substantial, well-documented extension to the minified-bundle static resolver in inventory.py (call-with-arguments follow-through with parameter shadowing, module-scoped identifier resolution for the ~2,100-module bundle, ||/?? fallback and .join() array handling), plus corresponding fixes in discover.py (PascalCase name splitting, user_facing_name scoring) and native_drift.py (filing unresolved descriptions as drift items). Verification claims (test counts, before/after unresolved-description counts, timing) are author-reported and not independently re-run in this review — I did not have execution access in this lane.

One finding posted inline, worth the author's attention but not necessarily blocking:

native_drift.py (L687-693) — the new unresolved-description filing treats a missing/unknown previous summary (prev is None, a true baseline run) as "every current name is new," unconditionally filing one item per currently-unresolved description. This is inconsistent with:

• This same function's new_candidates handling just above it, which explicitly guards "unknown prior state" to file nothing new (knows_candidates).
• The module's own documented baseline convention ("a missing previous summary is a baseline run: no surface diff, no new candidates...").

There is test coverage for "an old-shape previous summary (missing the field) files every name once" via a non-None self.prev, which suggests this might be an intentional onboarding choice — but there's no test for the true prev is None case with a non-empty unresolved_descriptions, and the asymmetry with new_candidates isn't called out anywhere. Worth either aligning the baseline behavior with new_candidates, or explicitly documenting/testing why it should differ. Full detail and a repro in the inline comment.

Everything else reviewed (module-scoped resolution rules, PascalCase splitting, SINGLE_TOKEN_FACTOR/described gating, shape validation for the new unresolved_descriptions field, template/fallback/array-join parsing) held up under inspection and appears consistent with its stated design and existing test patterns.

Branch: fix/inventory-resolve-tool-descriptions

Claude finished @kyle-sexton's task in 1m 21s —— View job

Security review — PR #5619

• Gather context (PR diff, changed files)
• Run /review:security-review for PR fix(claude-ops): resolve built-in descriptions the inventory left unresolved #5619
• Post findings

No security issues found.

Reviewed all 10 changed files (diff scoped to plugins/claude-ops/, base 54f3fe6^ through head b32c708):

• inventory.py — static-analysis parsing logic over the locally installed Claude Code CLI bundle (call-with-arguments follow-through with parameter shadowing, module-scoped identifier resolution across the ~2,100-module bundle, ||/?? fallback and .join() array handling, plus this commit's fixes for a statically-resolvable fallback left operand and for forward-declared value bindings). No subprocess/eval/exec/network calls; file I/O is limited to reading the local bundle and config paths from argparse, not untrusted remote input. The new/changed regexes (_LINK_RE, _binding_pattern, etc.) are linear character-class patterns, not susceptible to catastrophic backtracking.
• discover.py — adds PascalCase word-splitting (CAMEL_RE) and an extra scored text field (user_facing_name) to the TF-IDF bag used for overlap detection. Pure text scoring, no new I/O or trust-boundary crossing.
• native_drift.py — adds an unresolved-description drift item derived from integrity.undetermined.description_unresolved, filed through the existing key-based path with no label. No new external input handling. (Note: a functional/correctness asymmetry in this logic — baseline runs with prev is None file every unresolved name instead of nothing, unlike the sibling new_candidates handling — was already flagged by the code-review pass above; it has no security implication so is not repeated here.)
• extraction.md, native-drift.md, CHANGELOG.md, plugin.json — documentation and version-bump only.
• test_*.py — test coverage only.

None of the changed files are CLAUDE.md/AGENTS.md/rules/skill/agent-body files carrying standing instructions, so the instruction-surface-deletion check doesn't apply. No injection surfaces (command/SQL/path/template), no credential/token handling, and no authorization/access-control logic in this diff.

Branch: fix/inventory-resolve-tool-descriptions

Claude finished @kyle-sexton's task in 5m 5s —— View job

Code review complete

• Gather context (PR diff, prior Codex/Claude review comments, commit history)
• Diff the latest commit (b32c708, "address Codex review of the description resolver") against the previously reviewed head (337bf91)
• Verify each prior Codex finding was actually fixed, with regression coverage
• Review the new module-boundary/forward-reference logic (_declaration, _binding_value, _in_function_body) for correctness
• Post findings

Summary

Reviewed origin/main...HEAD (10 files) via gh pr diff, focused on what changed since the last review round (337bf91 → b32c708), since the four Codex findings from that round were the open items.

All four prior Codex findings are fixed, each with a new regression test:

• Scope single-character bindings to their module — _binding_value now rejects a single-letter binding found before the current module's start (v < lo), closing the // @bun-boundary crossing. Covered by test_a_single_letter_binding_never_crosses_a_module_boundary.
• Reject local value assignments that occur after the read — _declaration now takes a later_ok predicate; _binding_value only allows a forward-found var binding when the reader is inside an actual function/arrow body (_in_function_body), while _function_body keeps unconditional forward lookup (correct, since JS function declarations are fully hoisted, unlike var initializers). Covered by test_a_later_binding_resolves_only_inside_a_function_body.
• Preserve a statically resolved fallback left operand — _operand's ||/?? handling now records the left side's value(s) and only reads the right side when the left isn't unconditionally truthy. Covered by test_a_resolved_left_operand_wins_over_its_fallback.
• Add a verification record for the new bundle claims — extraction.md now carries the four-part claim/basis/as-of/recheck-trigger table for the ~2,100-module count and the "design is the only unresolved description" claim, matching this repo's skill-bodies-state-current-rules.md convention.

One new finding on the fix itself, posted inline on plugins/claude-ops/skills/inventory/scripts/inventory.py line 1342: _in_function_body's heuristic ("enclosing { preceded by ) or =>") also matches a top-level if/for/while/switch/catch block, not just a function/arrow body. Such a block at module scope runs synchronously during module init — the same timing the fix was built to exclude — so a var reference inside a top-level conditional wrapping a registration call could still forward-resolve to a value that isn't actually assigned yet at that point in execution. I traced this statically (this lane has no code-execution access, so it's unverified by a run) with a minimal repro in the inline comment; it's the same bug class as the one just fixed, reopened for control-flow blocks instead of bare object literals.

Not re-raised: the earlier open question about native_drift.py's diff() filing every currently-unresolved name on a true prev is None baseline run (asymmetric with new_candidates's "unknown prior state ⇒ nothing new" handling). This wasn't touched in the latest commit, but native-drift.md's own table entry for unresolved-description ("every listed name when <prev> recorded no list") and the native_drift.py docstring both now state this as intended behavior rather than an oversight — so it reads as a deliberate onboarding choice, not a bug. The only gap left is a direct test for the true prev=None (vs. old-shape non-None prev) case, which is a minor coverage nit rather than a blocking issue.

Verification claims in the PR body (test counts, before/after unresolved-description counts, timing) are author-reported and not independently re-run here — this lane has no execution access.
· Branch

@codex review

@codex review

Claude finished @kyle-sexton's task in 1m 29s —— View job

Security review — PR #5619

• Gather context (PR diff, changed files, prior reviews)
• Run /review:security-review for PR fix(claude-ops): resolve built-in descriptions the inventory left unresolved #5619
• Post findings

No security issues found.

Reviewed the full diff (origin/main...HEAD, head b445068) against the criteria in /review:security-review: injection surfaces, authorization/access-control logic, credential/token handling, and instruction-surface deletions.

• inventory.py — the bulk of the diff is static-analysis parsing logic over the locally installed Claude Code CLI bundle: call-with-arguments follow-through with parameter shadowing, module-scoped identifier resolution across the // @bun-delimited bundle, ||/?? fallback handling (this commit's own fix keeps a statically-resolved ?? empty string rather than folding it into || semantics), .join() array parsing, and deferred-flag threading that replaces the prior head's _in_function_body brace-shape heuristic with explicit call-graph propagation from getter/method/arrow read sites. All of this operates on an in-memory string already read by existing code paths — no new file I/O, subprocess, eval/exec, deserialization, or network calls. The new regexes (_LINK_RE, _binding_pattern, _function_pattern, CAMEL_RE) are linear character-class patterns over a trusted local input (the operator's own installed binary), not attacker-supplied data, so this isn't a meaningful ReDoS surface.
• discover.py — adds PascalCase word-splitting and an extra scored text field (user_facing_name) to the TF-IDF bag used for overlap detection. Pure text scoring, no new I/O or trust-boundary crossing.
• native_drift.py — adds an unresolved-description drift item derived from the extraction's integrity.undetermined.description_unresolved, filed through the existing key-based path with no label. No new external input handling or authorization logic.
• extraction.md, native-drift.md, CHANGELOG.md, plugin.json — documentation and version-bump only; none is an AGENTS.md/CLAUDE.md/rules/skill-or-agent-body guardrail, and nothing in the diff deletes, narrows, or softens a standing instruction (docs/conventions/instruction-exception-register/README.md was checked and doesn't apply here — these are parser-behavior reference docs, not operator-facing rules).
• test_*.py — test coverage only.

This confirms the two prior security-review passes on this PR (head 337bf91 and b32c708), which also found no issues; the intervening commit (b445068, "take later bindings only on deferred reads; keep ?? strings") only refines existing parsing logic in the same non-security surface.
· Branch

Claude finished @kyle-sexton's task in 3m 27s —— View job

Code review complete

• Gather context (PR diff, changed files, prior Codex/Claude review rounds)
• Invoke /review:code-review criteria for PR fix(claude-ops): resolve built-in descriptions the inventory left unresolved #5619
• Diff the latest commit (b445068, "take later bindings only on deferred reads; keep ?? strings") against the previously reviewed head (b32c708)
• Verify the two open Codex P2 findings from the prior round are actually fixed, with correct logic and regression coverage
• Post findings

Summary

Reviewed origin/main...HEAD (10 files) via gh pr diff, focused on what changed in the latest commit since the last review round (b32c708 → b445068), since two Codex findings from that round were the only open items:

Both are fixed correctly, each with a regression test:

• Distinguish nullish (??) from falsy (||) fallbacks — inventory.py#L1003-L1007 now narrows the "fall through to the right operand" check to src.startswith("||", i) and not all(values), so ?? only falls through on a truly missing value (None), not on a statically-resolved empty string. Traced description:""??"FALLBACK" by hand: values == [""], not values is False, and the ||-only guard is False, so the left "" is kept and recorded — matching JS's actual nullish-coalescing semantics. Covered by test_a_nullish_fallback_keeps_an_empty_string.
• Do not treat an eagerly-called function as deferred — rather than patching the previous brace-heuristic (_in_function_body, which this commit removes entirely), the fix threads an explicit deferred: bool parameter through _scan/_operand/_read_string/_resolve_chain/_binding_value. It's set True only where JS actually defers execution: inside a getter/method body (inventory.py#L1509-L1520), inside any arrow-function body (inventory.py#L1198, unconditional — correct, since code inside an arrow body never runs at the point the arrow literal is written), or when following a bare function-valued identifier read as a getter-like field (len(chain) == 1 in _resolve_chain). An explicit call like dd() at a non-deferred site keeps whatever deferred value it inherited, so description:dd() as a plain (non-method, non-getter) object-literal value — which _eval_field's "value" branch scans with deferred defaulting to False (inventory.py#L1522-L1523) — correctly stays non-deferred, so a var zz= binding written later in the module is no longer forward-resolved into it. This also incidentally closes the control-flow-block variant of the same bug I'd flagged on the prior commit (a top-level if/for/etc. wrapping an eager call): since the new design no longer inspects brace structure at the read site at all, that false-positive class can't recur. Covered by test_an_eager_call_does_not_read_a_later_binding; the getter/method/arrow/function-field deferred paths are separately covered by test_a_later_binding_resolves_only_inside_a_function_body.

Also in this commit: native_drift.py's baseline-run behavior (every currently-unresolved name filed as an unresolved-description item when prev is None, asymmetric with new_candidates's "unknown prior state ⇒ nothing new") is now explicitly documented as intentional (native-drift.md, the diff() docstring) and has a direct test for the true prev=None case — closing the gap I'd noted in an earlier round as "worth documenting or testing."

No new findings. I did not find additional correctness issues in this commit's diff. Static trace only — this lane has no code-execution access, so test_inventory.py/test_native_drift.py pass counts and the author's before/after unresolved-description figures are author-claimed and not independently re-run here.

Branch: fix/inventory-resolve-tool-descriptions

@codex review

Claude finished @kyle-sexton's task in 2m 45s —— View job

Code review complete

• Gather context (PR diff, changed files, prior review rounds)
• Load /review:code-review skill and instructions
• Confirm no functional commits landed after b3be945 — dd2cfb0 only merges in an unrelated origin/main feature (4e49cab), so the three P2 findings Codex raised against b3be945bbd on 2026-09-30T18:57:34Z were still live at this head
• Independently trace all three against the current inventory.py
• Post findings

Summary

Scoped to the 10 changed files (origin/main...HEAD). Most of this PR (the module-boundary resolution, deferred-read tracking, ||/?? nullish-vs-falsy split, argument-to-parameter binding, discover.py PascalCase splitting, and native_drift.py drift intake) has already been through several review rounds on this thread and I don't have new findings there — those all held up under inspection.

The one gap: three P2 findings Codex posted against b3be945bbd (2026-09-30T18:57:34Z) are still unaddressed at HEAD (dd2cfb0) — the only commit since b3be945 is a merge that pulls in an unrelated feature, not a fix. I independently re-traced each one against the current source (static trace only, no execution access in this lane) and confirmed all three are still reachable bugs. Posted as inline comments, each with a fix link:

1. inventory.py:884 — the empty-fallback check (src[i:i+2] in ('""', "''", "\`")) is a 2-character prefix test on the fallback's source text, not a check of what the whole right operand evaluates to. description:x||""+"Fallback"parses asx || ("" + "Fallback") in JS (+binds tighter than||), but the prefix test sees ""and discards the entire right side, losing the real"Fallback"` value.
2. inventory.py:1029 — computed is set True for any parenthesized group or call on the left of ||/??, regardless of whether it fully resolved to non-empty strings. (flag?"A":"C")||"B" resolves the left side completely (["A","C"], both truthy), but computed=True still forces the fallback "B" to be read and recorded as a spurious, runtime-unreachable variant.
3. inventory.py:1164 — a template substitution's resolved variants are collapsed to value[-1], discarding every other value. `Use ${flag?"X":"Y"}` only ever yields "Use Y", silently dropping the "Use X" variant — inconsistent with the +-concatenation path a few lines up in this same commit, which correctly preserves every combination via itertools.product.

None of these are newly introduced by the merge commit — they're latent in logic added by b3be945 and were already flagged by Codex; they just never got a follow-up fix commit like the earlier rounds' findings did. Author-reported test/verification claims elsewhere in the PR body are unverified by this lane (no execution access).

Branch: fix/inventory-resolve-tool-descriptions

Claude finished @kyle-sexton's task in 57s —— View job

Security review — PR #5619

• Gather context (PR diff, changed files)
• Run /review:security-review for PR fix(claude-ops): resolve built-in descriptions the inventory left unresolved #5619
• Post findings

No security issues found.

Reviewed all 10 changed files (origin/main...HEAD, head dd2cfb0) against the criteria in /review:security-review: injection surfaces, authorization/access-control logic, credential/token handling, and instruction-surface deletions.

• inventory.py — the bulk of the diff extends the static-analysis parser over the locally installed Claude Code CLI bundle: call-with-arguments follow-through with parameter shadowing and now argument binding (_bound_arguments), module-scoped identifier resolution across the // @bun-delimited bundle (_declaration, _chunk_imports, _export_index), ||/?? fallback handling, [...].join(sep) array parsing, and deferred-flag threading for forward binding lookups. All of this operates on an in-memory string already read by existing code paths (the operator's own installed binary) — no new file I/O, subprocess, eval/exec, deserialization, or network calls. The new regexes (_LINK_RE, _binding_pattern, _function_pattern, CAMEL_RE) are linear character-class patterns over trusted local input, not attacker-supplied data, so this isn't a meaningful ReDoS surface.
• discover.py — adds PascalCase word-splitting (CAMEL_RE) and an extra scored text field (user_facing_name) to the TF-IDF bag used for overlap detection. Pure text scoring, no new I/O or trust-boundary crossing.
• native_drift.py — adds an unresolved-description drift item derived from the extraction's integrity.undetermined.description_unresolved, filed through the existing key-based path with no label. No new external input handling or authorization logic. (The baseline-filing asymmetry with new_candidates that earlier review rounds flagged is a correctness/design question, not a security issue, and is now documented as intentional.)
• extraction.md, native-drift.md, CHANGELOG.md, plugin.json — documentation and version-bump only; neither is a CLAUDE.md/AGENTS.md/rules file or a SKILL.md/agent body carrying standing instructions (both are reference//context/ docs), so the instruction-surface-deletion check doesn't apply, and nothing here deletes, narrows, or softens a standing instruction.
• test_*.py — test coverage only.

This confirms the four prior security-review passes already posted on this PR across earlier commits, which also found no issues; this pass covers the full current head (dd2cfb0, the merge of origin/main into the branch) and finds nothing new.

Branch: fix/inventory-resolve-tool-descriptions

@codex review

@codex review

Claude finished @kyle-sexton's task in 1m 4s —— View job

Security review — PR #5619

• Gather context (PR diff, changed files, extensive prior review history — ~55+ rounds already on this thread, all clean)
• Invoke /review:security-review and follow it exactly for HEAD 121806d
• Post findings

No security issues found.

This PR has been through many clean security-review passes on prior heads (337bf918 through 1cbce94b40). This pass covers everything since the last reviewed head (1cbce94b406cdea69511487755270472131a38d2) through the current HEAD (121806d).

The only commit in that window is 121806d ("shadowing by bare declarations, catch params, quoted heads"), touching exactly two files:

• inventory.py — continues the same static-analysis-parser refinement pattern reviewed across every prior round, with four changes:
  • _opens_function now masks quoted text (via the already-reviewed _mask_strings) before the balanced-paren walk, so a parameter default containing a quoted paren (sep=")") no longer defeats the nested-function-detection fix from the previous commit. It now also bounds its scan to the enclosing brace or a 4096-byte window and raises ValueError on an unmatched head (propagating as "unresolved," the project's established fail-safe direction) rather than silently misclassifying.
  • _unset_between and the extended _binding_value add detection for a bare let x;/var a,x re-declaration or a catch(x) parameter shadowing a binding, correctly reporting "unresolved" instead of a stale value. Pure control-flow/regex bookkeeping over the same in-memory, trusted local input (the operator's own installed Claude Code CLI bundle) — no new file I/O, subprocess, eval/exec, deserialization, or network calls.
  • _write_pattern widens the destructuring-write detection from [^\[\]{};]* to a lazy [^;]*? bounded by the statement's ; terminator, so nested destructuring ({a:{b:x}}=) is caught. This is a single, non-nested lazy quantifier over a ;-bounded span — no alternation or nested-quantifier structure that could cause catastrophic backtracking, and the scanned text is the same trusted local source as every other regex in this file.
  • _declaration's home-module lookup edge case (home <= found.start() < at) is a narrow correctness refinement to where "re-declared/unset" detection starts scanning from; no new external input handling.
  • All of this still operates on the operator's own locally installed, trusted Claude Code CLI bundle — not attacker-supplied data — consistent with every prior commit on this PR.
• test_inventory.py — regression coverage only (4 new test cases).

No other files changed in this window. discover.py, native_drift.py, and the documentation/version-bump files (extraction.md, native-drift.md, CHANGELOG.md, plugin.json) are unchanged since their prior clean security-review passes. No injection surfaces (command/SQL/path/template), no credential/token handling, and no authorization/access-control logic anywhere in this diff.

The PR's full changed-file set (10 files) remains confined to plugins/claude-ops/, consistent with every prior pass on this thread — the functional surface throughout is a static-analysis parser over a locally installed, trusted CLI bundle, with no subprocess, eval/exec, deserialization, network calls, or attacker-controlled input at any point in this branch's history. None of the changed files are CLAUDE.md/AGENTS.md/rules/skill-or-agent-body files carrying standing instructions, so the instruction-surface-deletion check doesn't apply.

Branch ·

Claude finished @kyle-sexton's task in 6m 17s —— View job

Code review complete

• Gather context (PR diff, extensive prior review history — ~55+ rounds already on this thread)
• Load /review:code-review skill and follow its instructions exactly
• Diff HEAD 121806d against the previously reviewed head (1cbce94)
• Verify the open finding from the prior round (var-shadow across sibling blocks) and check for new issues
• Post findings (none survived — see summary)

Summary

The only commit since the last reviewed head (1cbce94) is 121806d ("shadowing by bare declarations, catch params, quoted heads"), touching inventory.py (+72/-21) and its test file (+38). It closes three remaining gaps:

• A declaration with no initializer, or a catch (x) parameter, shadows — the new _unset_between walks forward from the found binding to the reader, looking for a bare let x;/var a,x declarator (no = within the lookahead window) or a catch(x) parameter, and treats either as making the read's value non-static. Traced both new assertions in test_a_declaration_without_initializer_shadows (let xx;return xx and return xx;var xx) and test_a_catch_parameter_shadows by hand — both correctly resolve away from the stale "WRONG" outer binding.
• An imported binding is now checked for shadows only in the reader's own module — inventory.py#L1813-L1821 clamps _unset_between's search start to home (the reader's own module) whenever the resolved binding lives in a different module, so the new backward scan can't wander into unrelated foreign-module text.
• A nested function head with a quoted paren default is matched correctly — _opens_function now masks string/template contents before running _open_paren's depth-matching, so a default like sep=")" no longer desyncs the paren count, and raises ValueError (caught by resolve_field's existing broad except) when the head still can't be read, leaving that field unresolved rather than silently mis-scanning a nested function body as a branch of the caller. Traced test_a_nested_function_with_a_quoted_paren_default_is_not_a_branch — the masked scan correctly finds inner's own ( and excludes its body.
• Nested destructuring writes unbind a parameter — _write_pattern's destructuring alternative drops the old single-level [\[{][^\[\]{};]*ident...[\]}] shape (which couldn't match two closing brackets before =) for a lazy ident[^;]*?[\]}]\s*=, closing the gap for ({a:{b:x}}=...). Confirmed by hand-tracing test_a_nested_destructuring_write_unbinds_a_parameter.

Re-verified the previously open finding (my own comment on the 0065509/1c62471 rounds: "var shadow in a sibling block of the read's own wrapping block is still missed", since reader was computed as at's innermost brace rather than its enclosing function). That was actually already fixed two commits before the last reviewed head, in f1d3a04 (reader = _function_block(src, braces, at) instead of braces.enclosing(max(at-1,0))), which I'd missed re-confirming in the intervening rounds — current source at 121806d still has that fix (inventory.py:1706), and _redeclared_later's own _function_block-based comparison (used for the single-character identifier path too) independently covers the same case. No regression here.

Two non-blocking precision notes, both fail-safe (can only turn a real value into unresolved, never fabricate a wrong one), consistent with this PR's established "prefer unresolved over wrong" design — not raising as blocking:

• The catch(x) shadow check (_unset_between's _visible(braces, pos, at) without the src argument) treats the parameter as shadowing anywhere in the enclosing block, not just inside the catch body — so a read after the try/catch statement in the same function would also be (incorrectly) treated as shadowed. The new test only asserts assertNotEqual(..., "WRONG"), not a specific resolved value, which is consistent with this being a known, accepted looseness rather than an oversight.
• The broadened destructuring-write regex (ident[^;]*?[\]}]\s*=) no longer requires a bracket to actually wrap ident on the left, so an unrelated bracket-assignment later in the same statement (e.g. foo(x) + (obj[key] = val)) would also mark x as "written." Same safe direction as above (downgrades to unresolved, never produces a wrong string).

Not independently re-run: this lane has no code-execution access (python3/py_compile were denied by the sandbox) — this is a static trace only, consistent with every prior round on this PR. The author's test-run and before/after figures in the PR body are author-reported and not independently verified here.

No new findings. Every finding raised across this PR's ~55+ review rounds has been fixed with regression tests and independently re-verified, and this round's commit closes the three remaining open items with no new issue found.

Branch ·

@codex review

Claude finished @kyle-sexton's task in 5m 16s —— View job

Code review complete

• Gather context (PR diff, extensive prior review history — ~55+ rounds already on this thread)
• Load /review:code-review skill and follow its instructions exactly
• Diff HEAD e4b2d08 against the previously reviewed head (121806d)
• Confirm the merge commit introduced nothing new, and trace the one new functional commit's logic
• Post findings (one, inline)

Summary

The only functional commit since the last reviewed head (121806d) is a649742 ("loop-scoped for-head bindings and quoted parameter heads"), touching inventory.py (+77/-22) and its test file (+23). The second commit, e4b2d08, is a merge of origin/main whose only non-ancestor content is 55b880f (test temp-dir cleanup, unrelated) and disk-hygiene plugin changes entirely outside this PR's claude-ops scope — confirmed via git show --stat, no overlap with any file this PR owns.

a649742 fixes three gaps:

• A let/const in a for (...) head is scoped to the loop only. New _for_scope/_statement_start compute the loop's span (from the head's ( to the end of a braced body) and _visible consults it before falling back to the plain block check. Traced for(let xx="LOCAL";;){break}return xx by hand: the loop's xx is correctly excluded once the reader is past the loop's closing brace, so the read resolves to the outer var xx="REAL" instead — matches test_a_for_head_binding_ends_with_its_loop.
• Method/function parameter lists are matched with quoted text masked (_head_open). _opens_function and the new _params_before(src, braces, body_open) both route through _head_open, which runs _mask_strings over the candidate span before depth-counting the matching (. Traced description(xx,a="("){return xx}: previously the quoted "(" would desync the backward paren-depth count and drop xx from the parsed parameter list (so it wouldn't be shadowed); now the quote is masked first, xx is correctly extracted as a parameter name. Matches test_a_quoted_paren_default_keeps_a_method_parameter.
• A function declaration whose parameter list holds parentheses leaves same-named calls unresolved rather than silently picking a different declaration. _function_body now counts `function ident(` occurrences against _function_pattern(ident) matches (which can't span nested parens) in the same module; a mismatch returns None (unresolved) instead of letting the simpler-but-wrong declaration win. Traced function helper(){return"WRONG"} outer vs. a nested function helper(x=g()){return"REAL"}: the counts mismatch (2 vs. 1), so helper() resolves to nothing rather than the real JS answer (hoisted-nested-shadows, i.e. "REAL") or the wrong outer value — consistent with this PR's established "prefer unresolved over wrong" design, and matches test_a_function_with_a_call_default_hides_no_outer_one (which only asserts != "WRONG", not == "REAL", for exactly this reason).

One finding, posted inline on inventory.py:1580: _for_scope's if not start or ... treats _statement_start's legitimate 0 return value (a real offset, when the statement begins at the very start of its 4 KiB lookback window) the same as its None "not found" sentinel — inconsistent with the sibling _statement_keyword a few lines above, which correctly tests start is None. If triggered, this would make _for_scope miss a genuine for-head and fall back to unscoped visibility for that loop variable — the exact over-broad-visibility bug this commit closes, just reopened for a narrow corner (a for loop literally at the start of the lookback window). Real-world likelihood is very low given the 2.1.285 bundle's module headers, but the fix is a one-line, zero-risk change (start is None). Full repro and fix suggestion in the inline comment.

Not independently re-run: python3 -m unittest was denied by the sandbox in this pass, consistent with every prior round on this PR — this is a static trace only. The author's test-run and before/after figures in the PR body are author-reported and not independently verified here.

Not re-raised: everything else fixed across this PR's ~55 review rounds (module-boundary resolution and visibility, deferred-read tracking, ||/?? nullish-vs-falsy handling, numeric/unary/primitive literal settlement, argument-to-parameter binding, template/join/concatenation alternative preservation, hoisted-function shadowing and lexical-scope declaration ranking, object-alias following, var-shadow detection, discover.py PascalCase splitting, native_drift.py drift intake) is unaffected by this commit and remains fixed.

Branch ·

Claude finished @kyle-sexton's task in 1m 27s —— View job

Security review complete

• Gather context (PR diff, changed files, extensive prior review history — ~55+ rounds already on this thread, all clean)
• Invoke /review:security-review and follow it exactly for HEAD e4b2d08
• Post findings

No security issues found.

This PR has been through many clean security-review passes on prior heads (337bf918 through 121806dd6e). This pass covers everything since the last reviewed head (121806dd6ecffce38a1b2967c9874e435b670012) through the current HEAD (e4b2d08a9d9a9fc11256cb802d6b2e8cb079b254).

Two commits landed in that window:

• a649742 ("loop-scoped for-head bindings and quoted parameter heads") — the only functional change, touching exactly two files:
  • inventory.py — continues the same static-analysis-parser refinement pattern reviewed across every prior round:
    • _head_open (replacing the inline paren-matching previously duplicated in _opens_function/_params_before) matches a parameter list's ( against quoted text blanked via the already-reviewed _mask_strings, bounded to the enclosing block or a 4096-byte window (inventory.py#L641-L646).
    • _for_scope/_statement_start (inventory.py#L1570-L1596) determine the visibility span of a let/const declared in a for(...) head. _statement_start is the pre-existing 4096-byte-bounded backward walk (unchanged logic, just extracted into its own function); _for_scope's own regex searches are a 16-byte lookback (for\s*(?:await\s*)?$) and reuse of _match_close/braces.pairs — all bounded, linear, non-backtracking-prone patterns over the same trusted local input (the operator's own installed Claude Code CLI bundle, not attacker-supplied data).
    • _function_body (inventory.py#L1858-L1868) adds a count comparison between a loose function NAME( scan and the strict _function_pattern to detect when a parameter list containing parens hides a real declaration, and conservatively returns None (unresolved) rather than guessing — consistent with this PR's established fail-safe design.
    • All of this is pure, bounded, in-memory string/regex analysis over data already read by existing code paths — no new file I/O, subprocess, eval/exec, deserialization, or network calls, and no unbounded or nested-quantifier regex that could introduce a ReDoS regression.
  • test_inventory.py — regression coverage only.
• e4b2d08 (merge of origin/main) — its only non-a649742 parent, b906919 (feat(disk-hygiene): ..., feat(disk-hygiene): catalog scope, ownership investigation and cross-target answer reuse #5630), is an unrelated, already-merged feature with no overlap with any claude-ops file — confirmed via git show --stat.

plugin.json/CHANGELOG.md changes are version-bump (0.77.4 → 0.78.0) and changelog text only, no behavioral change. discover.py, native_drift.py, and the remaining documentation files are unchanged since their prior clean security-review passes.

Full diff against the merge-base with origin/main (git diff b9069194eb..HEAD) remains exactly the PR's stated 10-file set (+1902/-86), confined entirely to plugins/claude-ops/ — the functional surface throughout is a static-analysis parser over a locally installed, trusted CLI bundle, with no subprocess, eval/exec, deserialization, network calls, or attacker-controlled input at any point in this branch's history. No injection surfaces (command/SQL/path/template), no credential/token handling, and no authorization/access-control logic anywhere in this diff. None of the changed files are CLAUDE.md/AGENTS.md/rules/skill-or-agent-body files carrying standing instructions, so the instruction-surface-deletion check doesn't apply.

Branch ·

@codex review

Claude finished @kyle-sexton's task in 1m 40s —— View job

Security review — PR #5619

• Gather context (PR diff, changed files, extensive prior review history on this thread — ~55+ clean passes across heads 337bf918 through e4b2d08)
• Invoke /review:security-review and follow it exactly for HEAD 9be6b2a
• Post findings

No security issues found.

This PR has been through many clean security-review passes on prior heads (337bf918 through e4b2d08a9d). This pass covers everything since the last reviewed head (e4b2d08a9d9a9fc11256cb802d6b2e8cb079b254) through the current HEAD (9be6b2a1863406447d6e32516d183cf7bb81a4fe).

Two commits landed in that window:

• 0a0c8a7 ("catch parameters shadow bound ones; masked control heads") — the only functional change, touching exactly two files:
  • inventory.py#L639-L651 — new _catch_params extracts a catch (...) block's parameter name(s) so they're added to the shadow scope, closing a gap where a followed function's bound parameter could leak into a catch block reusing the same name. It calls the already-reviewed _head_open (quote-masked paren matching) and a 16-byte-bounded regex lookback (re.search(r"(?<![\w$.])catch\s*$", src[max(0, k-16):k])) — a small, fixed-size, non-backtracking-prone scan over the operator's own locally installed, trusted Claude Code CLI bundle, not attacker-supplied data.
  • inventory.py#L1573-L1576 — _function_block now calls _head_open (which masks quoted text before matching parens) instead of the old _open_paren, wrapped in a try/except ValueError that falls back to treating the position as a plain block on an unmatched head. This fixes a correctness gap (if(a==="("){...} previously could be misread) and is a pure control-flow/exception-handling change — no new I/O, subprocess, eval/exec, deserialization, or network calls.
  • test_inventory.py — regression coverage only (test_a_catch_parameter_shadows_a_bound_parameter, test_a_quoted_paren_in_a_control_head_keeps_a_var_function_scoped).
• 9be6b2a (merge of origin/main) — its other parent, 8a159a4 (fix(disk-hygiene): accept braces inside quoted words in the engine gate, fix(disk-hygiene): accept braces inside quoted words in the engine gate #5644), is an unrelated, already-merged fix with no overlap with any claude-ops file — confirmed via git diff e4b2d08..9be6b2a --stat -- plugins/claude-ops/, which shows only the two files above changed.

No other files changed in this window. discover.py, native_drift.py, and the documentation/version-bump files (extraction.md, native-drift.md, CHANGELOG.md, plugin.json) are unchanged since their prior clean security-review passes. No injection surfaces (command/SQL/path/template), no credential/token handling, and no authorization/access-control logic anywhere in this diff.

Confirmed the full PR diff (git diff origin/main...HEAD) still matches the PR's stated shape — 10 files, +1937/-86, confined entirely to plugins/claude-ops/. The functional surface throughout this branch's history is a static-analysis parser over a locally installed, trusted CLI bundle, with no subprocess, eval/exec, deserialization, network calls, or attacker-controlled input at any point. None of the changed files are CLAUDE.md/AGENTS.md/rules/skill-or-agent-body files carrying standing instructions, so the instruction-surface-deletion check doesn't apply.

Branch ·

Claude finished @kyle-sexton's task in 3m 28s —— View job

Code review complete

• Gather context (PR diff, extensive prior review history — ~60 rounds already on this thread)
• Load /review:code-review skill and follow its instructions exactly
• Diff HEAD 9be6b2a against the previously reviewed head (e4b2d08)
• Check for any new issues introduced since the last review round
• Post findings (none survived — see summary)

Summary

Two commits landed since the last reviewed head (e4b2d08):

• 0a0c8a7 ("catch parameters shadow bound ones; masked control heads") — the only functional commit, touching inventory.py (+23/-3) and its test file (+18).
• 9be6b2a (merge of origin/main) — its only non-ancestor content is 8a159a4 (fix(disk-hygiene): accept braces inside quoted words in the engine gate, fix(disk-hygiene): accept braces inside quoted words in the engine gate #5644), entirely unrelated to claude-ops — confirmed via git show --stat, no overlap with any file this PR owns. git diff origin/main...HEAD still matches the PR's stated 10-file/+1937/-86 shape.

0a0c8a7 closes two gaps:

• A catch (x) parameter shadows a bound parameter of the same name. _catch_params reuses the new _head_open to find the catch head's parameter list and is merged into the shadow scope (inventory.py#L1024) whenever a scanned block turns out to be a catch(...) block — shadow | _catch_params(...) lets the catch parameter win over an outer bound argument of the same name, since dict-union prefers the right operand. Traced the new test (function ff(x){try{throw"LOCAL"}catch(x){return x}} called as ff("REAL")): the catch's own x (a runtime value, None) now shadows the bound x=["REAL"] inside the catch body, so return x no longer resolves to the stale "REAL".
• A quoted paren in a control head no longer desyncs the function/block boundary walk. _function_block now calls _head_open (which masks string/template contents via _mask_strings before depth-matching parens) instead of the old raw-character _open_paren counter. Traced the new test (function ff(){if(a==="("){var xx="REAL"}return xx} with an outer var xx="WRONG"): the literal "(" inside the if condition no longer miscounts the paren depth, so _function_block correctly keeps walking up through the if block to ff's own body instead of mistaking the if-head for a function boundary — which in turn lets _redeclared_later's _function_block(pos) == reader comparison correctly detect the nested var xx as shadowing the outer one.

I additionally traced two correctness properties by hand (no execution access in this sandbox, consistent with every prior round on this PR):

• _opens_function and _catch_params both call _head_open(src, braces, j) with identical arguments when a block is preceded by ); since the computation is deterministic, if the first call (used in _scan's nested-block guard, inventory.py#L1013) doesn't raise ValueError, the second (_catch_params's own call) won't either — so this refactor doesn't introduce a new double-raise or inconsistent-match risk between the two call sites.
• _head_open's new bounding (lo = outer[0]+1 when an enclosing brace exists, else max(chunk_start, close-4096)) is strictly tighter than the old unbounded-by-braces 4096-byte _open_paren scan, so it can't regress a previously-correct match for well-formed code — it only prevents a match from crossing into an outer brace scope it shouldn't, and (via masking) prevents a quoted paren from corrupting the depth count.
• _function_block's except ValueError: return block preserves the old k is None: return block fallback behavior exactly, so the fail-safe direction (treat as hitting a function boundary when the head can't be read) is unchanged.

No new findings. Both new regression tests use assertNotEqual (the project's established "prefer unresolved/different over wrong" convention for this class of fix), and I didn't find a case where either fix overcorrects or reopens a previously-closed gap. Everything else fixed across this PR's ~60 review rounds (module-boundary resolution and visibility, deferred-read tracking, ||/?? handling, numeric/unary/primitive literal settlement, argument-to-parameter binding with reassignment/destructuring/compound-write exclusion, hoisted-function shadowing and lexical-scope declaration ranking, object-alias following with per-hop deferral, var-shadow detection across nested/sibling blocks, discover.py PascalCase splitting, native_drift.py drift intake) is unaffected by this commit and remains fixed.

Branch